Replace error-prone instanceof in Rules classes #3858

zonuexe · 2025-03-06T16:31:03Z

Reduce usage of $type instanceof *Type pattern in Rules classes.

zonuexe · 2025-03-06T16:33:24Z

src/Rules/TooWideTypehints/TooWideMethodReturnTypehintRule.php

-			&& !$isFirstDeclaration
+			!$isFirstDeclaration
+			&& !$method->isPrivate()
+			&& ($returnType->isNull()->yes() || $returnType->isTrue()->yes() || $returnType->isFalse()->yes())


Logical operators are short-circuit evaluated, so rearranging expressions can lead to micro-optimizations.

ondrejmirtes

I don't want to replace these calls by just accessing the first element. Union types should be properly handled. Multiple values should be iterated over.

src/Rules/Api/NodeConnectingVisitorAttributesRule.php

zonuexe · 2025-03-06T16:53:31Z

Since ConstantArrayType and ConstantStringType each return return [$this]; from the getConstant*() methods, I thought it would be effective to simply replace them with count() === 0 or count() === 1 to minimize the change difference, but I also prefer to replace it with foreach instead of $types[0], so I will change my approach and fix it that way.

zonuexe · 2025-03-07T18:47:31Z

src/Rules/PhpDoc/RequireExtendsCheck.php

-			$class = $type->getClassName();
-			$referencedClassReflection = $type->getClassReflection();
+			$classNames = $type->getObjectClassNames();
+			sort($classNames);


I am sorting this array because tests seem to fail randomly.

https://github.com/phpstan/phpstan-src/actions/runs/13726816592/job/38395109333?pr=3858

zonuexe · 2025-03-07T18:50:41Z

MissingTypehintCheck was reverted in this branch because the literal substitution seemed to change its behavior. a408a58

phpstan-bot · 2025-03-07T18:50:57Z

This pull request has been marked as ready for review.

staabm · 2025-03-08T07:44:15Z

src/Rules/PhpDoc/RequireExtendsCheck.php


-			$class = $type->getClassName();
-			$referencedClassReflection = $type->getClassReflection();
+			$classNames = $type->getObjectClassNames();


Introduce this var earlier so you don't need to call getObjectClassNames twice (methods called on Type can get expensive)

staabm · 2025-03-08T07:45:01Z

src/Rules/PhpDoc/RequireImplementsDefinitionTraitRule.php

 		foreach ($implementsTags as $implementsTag) {
 			$type = $implementsTag->getType();
-			if (!$type instanceof ObjectType) {
+			if (count($type->getObjectClassNames()) === 0) {


Introduce a var so you don't need to call getObjectClassNames twice

staabm · 2025-03-08T07:45:44Z

src/Rules/Variables/CompactVariablesRule.php

 	{
-		if ($type instanceof ConstantStringType) {
-			return [$type];
+		if (count($type->getConstantStrings()) > 0) {


Introduce a var so you don't need to call getConstantStrings twice

VincentLanglet · 2025-03-09T22:05:01Z

src/Rules/Arrays/DuplicateKeysInLiteralArraysRule.php

-				$arrayKeyValue = $keyType->toArrayKey();
-				if ($arrayKeyValue instanceof ConstantIntegerType) {
+				$arrayKeyValues = $keyType->toArrayKey()->getConstantScalarValues();
+				if (count($arrayKeyValues) === 1 && is_int($arrayKeyValues[0])) {


Wouldn't it better/possible to handle union of scalar values here (?)

Do you mean the following?

modified src/Rules/Arrays/DuplicateKeysInLiteralArraysRule.php @@ -71,10 +71,12 @@ final class DuplicateKeysInLiteralArraysRule implements Rule } else { $keyType = $itemNode->getScope()->getType($key); $arrayKeyValues = $keyType->toArrayKey()->getConstantScalarValues(); - if (count($arrayKeyValues) === 1 && is_int($arrayKeyValues[0])) { - $autoGeneratedIndex = $autoGeneratedIndex === null - ? $arrayKeyValues[0] - : max($autoGeneratedIndex, $arrayKeyValues[0]); + foreach ($arrayKeyValues as $arrayKeyValue) { + if (is_int($arrayKeyValue)) { + $autoGeneratedIndex = $autoGeneratedIndex === null + ? $arrayKeyValue + : max($autoGeneratedIndex, $arrayKeyValue); + } } }

Or is it type-level arithmetic?

modified src/Rules/Arrays/DuplicateKeysInLiteralArraysRule.php @@ -42,7 +42,6 @@ final class DuplicateKeysInLiteralArraysRule implements Rule $valueLines = []; /** - * @var int|false|null $autoGeneratedIndex * - An int value represent the biggest integer used as array key. * When no key is provided this value + 1 will be used. * - Null is used as initializer instead of 0 to avoid issue with negative keys. @@ -63,27 +62,37 @@ final class DuplicateKeysInLiteralArraysRule implements Rule } if ($autoGeneratedIndex === null) { - $autoGeneratedIndex = 0; - $keyType = new ConstantIntegerType(0); + $autoGeneratedIndex = new ConstantIntegerType(0); } else { - $keyType = new ConstantIntegerType(++$autoGeneratedIndex); + $autoGeneratedIndex = $scope->getType(new Plus( + new Int_(1), + new TypeExpr($autoGeneratedIndex), + )); } + $keyType = $autoGeneratedIndex; } else { $keyType = $itemNode->getScope()->getType($key); - $arrayKeyValues = $keyType->toArrayKey()->getConstantScalarValues(); - if (count($arrayKeyValues) === 1 && is_int($arrayKeyValues[0])) { + $arrayKeyType = $keyType->toArrayKey(); + if ($autoGeneratedIndex !== false && $arrayKeyType->isInteger()->yes()) { $autoGeneratedIndex = $autoGeneratedIndex === null - ? $arrayKeyValues[0] - : max($autoGeneratedIndex, $arrayKeyValues[0]); + ? $keyType + : $scope->getType(new FuncCall( + new Name('\max'), + [ + new Arg(new TypeExpr($autoGeneratedIndex)), + new Arg(new TypeExpr($arrayKeyType)), + ] + )); } }

$autoGeneratedIndex is explained as follows:

/** * - An int value represent the biggest integer used as array key. * When no key is provided this value + 1 will be used. * - Null is used as initializer instead of 0 to avoid issue with negative keys. * - False means a non-scalar value was encountered and we cannot be sure of the next keys. */ $autoGeneratedIndex = null;

Since the key value actually changes depending on the input value, more appropriate typing is possible with type operations. However, since DuplicateKeysInLiteralArraysRule has some subtle behavior, I would like to keep the changes in this branch to a minimum.

VincentLanglet · 2025-03-09T22:08:53Z

src/Rules/Comparison/ConstantLooseComparisonRule.php

 		};

-		if (!$nodeType->getValue()) {
+		if (in_array(false, $nodeType->getConstantScalarValues(), true)) {


I'm not sure but isn't $nodeType->isFalse()->yes() better ?

That's correct, thanks!

VincentLanglet · 2025-03-09T22:13:27Z

src/Rules/Comparison/ImpossibleCheckTypeHelper.php

-					$assertValue = ($this->treatPhpDocTypesAsCertain ? $scope->getType($arg) : $scope->getNativeType($arg))->toBoolean();
-					if (!$assertValue instanceof ConstantBooleanType) {
+					$assertValues = ($this->treatPhpDocTypesAsCertain ? $scope->getType($arg) : $scope->getNativeType($arg))->getConstantScalarValues();
+					if (count($assertValues) !== 1 || !is_bool($assertValues[0])) {


You could maybe have kept the toBoolean and checked

$assertValue->isTrue()->yes() || $assertValue->isFalse()->yes()

phpstan-bot · 2025-03-11T22:58:24Z

This pull request has been marked as ready for review.

VincentLanglet · 2025-03-12T08:29:37Z

src/Rules/Comparison/ImpossibleCheckTypeHelper.php

+					/** @var bool */
+					return $assertValue->getConstantScalarValues()[0];


You can return $assertValue->isTrue()->yes() if you want to avoid the var phpdoc

Makes sense!

ondrejmirtes · 2025-05-05T09:32:42Z

I'd love to merge this, but please fix the conflicts.

ondrejmirtes · 2025-05-05T14:29:22Z

I was able to rebase this myself 🎉

ondrejmirtes · 2025-05-05T14:35:42Z

Thank you.

zonuexe commented Mar 6, 2025

View reviewed changes

zonuexe marked this pull request as draft March 6, 2025 16:34

ondrejmirtes requested changes Mar 6, 2025

View reviewed changes

zonuexe force-pushed the refactor/error-prone-instanceof-type branch from bbbe15f to 776e845 Compare March 6, 2025 16:46

staabm reviewed Mar 6, 2025

View reviewed changes

src/Rules/Api/NodeConnectingVisitorAttributesRule.php Outdated Show resolved Hide resolved

zonuexe force-pushed the refactor/error-prone-instanceof-type branch 7 times, most recently from 9fc4e20 to c57fe20 Compare March 7, 2025 18:00

zonuexe commented Mar 7, 2025

View reviewed changes

zonuexe marked this pull request as ready for review March 7, 2025 18:50

staabm reviewed Mar 8, 2025

View reviewed changes

VincentLanglet reviewed Mar 9, 2025

View reviewed changes

zonuexe marked this pull request as draft March 11, 2025 16:26

zonuexe force-pushed the refactor/error-prone-instanceof-type branch 2 times, most recently from b031d1d to 5d96286 Compare March 11, 2025 21:12

zonuexe marked this pull request as ready for review March 11, 2025 22:58

VincentLanglet reviewed Mar 12, 2025

View reviewed changes

zonuexe force-pushed the refactor/error-prone-instanceof-type branch from 5d96286 to ddb9f66 Compare March 13, 2025 17:40

Replace error-prone instanceof in Rules classes

3e424f9

ondrejmirtes force-pushed the refactor/error-prone-instanceof-type branch from ddb9f66 to 3e424f9 Compare May 5, 2025 14:31

ondrejmirtes merged commit 551d3fd into phpstan:2.1.x May 5, 2025
411 of 416 checks passed

zonuexe deleted the refactor/error-prone-instanceof-type branch May 14, 2025 12:17

staabm mentioned this pull request May 14, 2025

"Array has duplicate keys with value" false-positives phpstan/phpstan#13013

Closed

		/** @var bool */
		return $assertValue->getConstantScalarValues()[0];

Replace error-prone instanceof in Rules classes #3858

Replace error-prone instanceof in Rules classes #3858

Conversation

zonuexe commented Mar 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zonuexe commented Mar 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zonuexe commented Mar 7, 2025

Uh oh!

phpstan-bot commented Mar 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phpstan-bot commented Mar 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ondrejmirtes commented May 5, 2025

Uh oh!

ondrejmirtes commented May 5, 2025

Uh oh!

Uh oh!

ondrejmirtes commented May 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants